-
Notifications
You must be signed in to change notification settings - Fork 351
Autolink mix tasks #911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autolink mix tasks #911
Conversation
}) == "[`mix hex.publish`](#{@elixir_docs}hex/Mix.Tasks.Hex.Publish.html)" | ||
end | ||
|
||
test "does not autolink task with arguments" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as the 1st step I skipped it but autolinking these too can be nice, see screenshot.
I don't think I need to extend that function because I'm just adding I'll update the type spec, good catch! |
|
||
defp re_kind_language(:mix_task, :elixir) do | ||
~r{ | ||
mix\ ([a-z]+[a-z0-9\._]*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we handle more than one white-space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep it as is, if someone accidentally adds more it won't highlight so it's gonna be easier to spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is that if a test editor wraps the line and we end up with
`mix
task argument`
or
`mix task
arguments`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eksperimental i think we can wait until it happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eksperimental i think we can wait until it happens.
@josevalim already happening
https://github.com/elixir-lang/elixir/pull/8372/files#diff-c12d92120ee0fe3d324a448c404f75a6L26
@wojtekmach I would also update the code comment:
|
thanks @fertapric!
kind <- [:module, :function] do | ||
{kind, language, link_type} | ||
end) | ||
end) ++ [{:mix_task, :elixir, :normal}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could also support :custom
links.
such as we do with the rest of the funtions.
ex_doc/test/ex_doc/formatter/html/autolink_test.exs
Lines 433 to 434 in 5e00c08
assert project_doc("[the :erlang.apply/2 function](`Kernel.apply/2`)", %{}) == | |
"[the :erlang.apply/2 function](#{@elixir_docs}elixir/Kernel.html#apply/2)" |
assert project_doc("[the :erlang.apply/2 function](`Kernel.apply/2`)", %{}) ==
"[the :erlang.apply/2 function](#{@elixir_docs}elixir/Kernel.html#apply/2)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fertapric convinced me that custom links can sometimes hurt IEx experience so how about we hold off on this change for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fertapric convinced me that custom links can sometimes hurt IEx experience so how about we hold off on this change for now?
Is that something that we could improve on the IEx side?
# `language` can be: `:elixir`, `:erlang` or `:markdown`. | ||
# | ||
# `kind` is either `:function` or `:module`. | ||
# `kind` is either `:function`, `:module`, or `:mix_task`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with :mix_task, but I'm wondering. Is there any other kind of task?
Should it be called just task
?
is there a equivalent of a Mix Task in Erlang?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are Rebar tasks which we might similarly support one day but not sure how hard that would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beacuse I'm seeing we are calling {:mix_task, :elixir, :normal}
, it is a bit redundant I guess.
{:mix_task, :elixir, :normal}
, and in the future we could call {:task, :erlang, :normal}
looks better IMO
My only concern is that there are other tools other than Mix and Rebar for creating tasks that we may support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for mix_task
because that's also what we call it in the sidebar.
@wojtekmach: It looks awesome at first glance, I would like to give it a try, tinker with it, and get back to you with more feedback if needed, but unfortunately I'll have to do this later. |
@eksperimental great, no rush! Also just wanted to say that it was very easy to add this functionality after your refactoring, thanks for that. |
@wojtekmach you are welcome! The refactoring came as a must do, after I spent so long trying to fix a bug, and I wasn't able to. |
easy: fix 1 bug, add 2 more, rinse and repeat, wait for a hero to clean up 😂 |
Co-Authored-By: wojtekmach <wojtek@wojtekmach.pl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only three minor corrections.
end | ||
end | ||
|
||
defp replace_fun(:mix_task, :elixir, _link_type, options) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`since we are only supporting :normal links, this line can read:
defp replace_fun(:mix_task, :elixir, :normal, options) do
|
||
defp re_kind_language(:mix_task, :elixir) do | ||
~r{ | ||
mix\ ([a-z]+[a-z0-9\._]*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the +
is redundant in the regex
end | ||
end | ||
|
||
describe "mix tasks" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mix tasks
Closes #910